Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove sybase use for star stats #337

Merged
merged 5 commits into from
Nov 20, 2019
Merged

Remove sybase use for star stats #337

merged 5 commits into from
Nov 20, 2019

Conversation

jeanconn
Copy link
Contributor

Remove sybase use for star stats

This is a redo of #257 and #187

@jeanconn
Copy link
Contributor Author

Regarding

#187 (comment)

I did compare this current approach (fetching all acq and guide stats and then using non-indexed searches within them) to using the agasc_id indexes on the hdf5 tables (via tables.read_where).

For an average set of stars in a schedule, it looks like using the indexed approach would reduce this part of the runtime by a bit more than a third (2.2 seconds to use the pre-fetch, 1.4 seconds to open the hdf5, keep it open, and do indexed queries). The 0.8 seconds did not seem worth it in this case (though fetching all the stats to start is not particularly memory efficient either).

@jeanconn
Copy link
Contributor Author

@taldcroft Do you want this to go too or would you like more time to consider?

@jeanconn jeanconn requested review from taldcroft and removed request for taldcroft November 18, 2019 17:43
Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! For testing I ran this on my Mac with JUN1019A and got reasonable results. As an extreme case abell 2146 obsid 20560 did show some diffs in the popup acq stats, but that might just be observations that happened between starcheck (flight) being run compared to the observation time which is what gets used for clipping results in the new version.

Is it an option (as a separate issue) to move most of the Python code into standalone modules in starcheck (the Python package)? That way you could put in unit testing and have your editor do PEP8 style checking etc.

One minor comment, I was just wondering if you actually need to make acqs and guides into astropy Table. I couldn't see anything that required this.

@jeanconn
Copy link
Contributor Author

IIRC acqs and guides were put into Table from this suggestion #257 (comment)

@jeanconn
Copy link
Contributor Author

I can add a comment about that if it isn't going to be the convention to just Table things if the code holds still long enough.

@taldcroft
Copy link
Member

Thanks for refreshing my memory. Indeed any data coming straight from HDF5 should generally be Table'd so that character data can be handled sensibly. So to answer my question, the string comparison ok = acqs['img_func'] == 'star' does require Table.

@taldcroft taldcroft self-requested a review November 20, 2019 21:01
Copy link
Member

@taldcroft taldcroft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants